Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed potential infinite recursion ISyncSimple crashes #424

Conversation

SokyranTheDragon
Copy link
Member

Calls to SyncSerialization.CanHandle can cause a crash due to infinite recursion when used with a subtype of ISyncSimple.

The crash would happen when a subtype of ISyncSimple would have a ISyncSimple field. The CanHandle checks all fields of a type implementing ISyncSimple, as well as all its subtypes, to see if they can be synced. However, as it encounters ISyncSimple during the check, it would then check if it can sync ISyncSimple, which would then check if it can sync ISyncSimple again and again until a crash.

I've originally attempted to fix this by modifying the conditions to skip in specific situations, like the type being an interface, or for recursively checking subtypes of self, etc. - this ended up very messy, and I would just end up finding another example that would cause a crash. I've opted out for this solution as it should handle all of the weird edge cases we may encounter.

Some simple examples of types that would cause a crash:

class TestSync : ISyncSimple
{
    ISyncSimple test;
}
class TestSync : ISyncSimple
{
    List<ISyncSimple> test;
}

@SokyranTheDragon SokyranTheDragon added fix Fixes for a bug or desync. 1.4 Fixes or bugs relating to 1.4 (Not Biotech). labels Feb 23, 2024
@Zetrith
Copy link
Member

Zetrith commented Feb 23, 2024

Good catch, though the issue is more subtle than that. Your solution lets the type ShouldntSync below pass:

class ShouldntSync : ISyncSimple
{
    SomeType test;
}

class SomeType : ISyncSimple
{
    NotSyncable test;
}

The calls to CanHandle on ISyncSimples can follow arbitrary graphs and the problem here is of cycle prevention. I'll fix it in a future update.

Where do you need to use generic ISyncSimple fields? The simplest fix for this case is to replace
All(f => CanHandle(f.FieldType)) with
All(f => f.FieldType == typeof(ISyncSimple) || CanHandle(f.FieldType)).

@SokyranTheDragon
Copy link
Member Author

Where do you need to use generic ISyncSimple fields?

My plan was to slightly modify the styling station patch so other mods could hook into it. Basically, I wanted to make a compat patch for Humanoid Alien Races (currently causes exceptions and breaks the dialog with MP due to dummy pawn not having all required HAR data), but I can't without slight changes to MP itself. So I've wanted to (among other things) add public List<ISyncSimple> additionalData to StylingData so mods could sync extra data from the dialog.

 

The simplest fix for this case is to replace
All(f => CanHandle(f.FieldType)) with
All(f => f.FieldType == typeof(ISyncSimple) || CanHandle(f.FieldType)).

I originally did something similar, but then I just kept finding more and more edge cases. For example, this won't handle the following:

// This is the one I rely on for the changes I planned to make
class TestSync : ISyncSimple
{
    List<ISyncSimple> test;
}
class Test : ISyncSimple
{
    Test test;
}
interface IExtendedSyncSimple : ISyncSimple
{
}

class Test : IExtendedSyncSimple
{
    IExtendedSyncSimple test;
}
// This example is where I finally gave up and made a simple solution
class TestFirst : ISyncSimple
{
    TestSecond test;
}

class TestSecond : ISyncSimple
{
    TestFirst test;
}

@Zetrith
Copy link
Member

Zetrith commented Mar 15, 2024

I think adding

if (type == typeof(ISyncSimple))
    return true;

before

if (typeof(ISyncSimple).IsAssignableFrom(type))

will suffice for now. Can you make this change?

@SokyranTheDragon
Copy link
Member Author

I'll try to make that change tomorrow, it's a bit late right now.

Matches the code before the changes. The brackets are no longer needed for the fix to work.
@SokyranTheDragon
Copy link
Member Author

I've made the changes. This should handle the most common issues, but could fail in more complex (and rare) situations.

@Zetrith
Copy link
Member

Zetrith commented Apr 5, 2024

Included in #431

@Zetrith Zetrith closed this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.4 Fixes or bugs relating to 1.4 (Not Biotech). fix Fixes for a bug or desync.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants